Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Generalize indexing AbstractUnitRanges with offset ranges #244

Merged
merged 1 commit into from
Jun 14, 2021
Merged

Generalize indexing AbstractUnitRanges with offset ranges #244

merged 1 commit into from
Jun 14, 2021
+129 −66

Conversation

jishnub
Copy link
Member

@jishnub jishnub commented Jun 13, 2021

The main change is to define getindex(::AbstractUnitRange, ::IdOffsetRange) instead of getindex(::UnitRange, ::IdOffsetRange), and similarly for IdentityUnitRange. This makes vector indexing preserve indices for custom unitrange types as well.

on v1.6

julia> Base.OneTo(4)[Base.IdentityUnitRange(2:3)]
2:3

julia> using OffsetArrays

julia> Base.OneTo(4)[Base.IdentityUnitRange(2:3)]
OffsetArrays.IdOffsetRange(values=2:3, indices=2:3)

julia> using StaticArrays

julia> SOneTo(3)[Base.IdentityUnitRange(2:3)]
OffsetArrays.IdOffsetRange(values=2:3, indices=2:3)

@codecov
Copy link

codecov bot commented Jun 13, 2021 •
edited
Loading

Codecov Report

Merging #244 (fd89ee3) into master (e9f33f0) will increase coverage by 0.06%.
The diff coverage is 100.00%.

❗ Current head fd89ee3 differs from pull request most recent head 50dc7fb. Consider uploading reports for the commit 50dc7fb to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #244      +/-   ##
==========================================
+ Coverage   95.19%   95.26%   +0.06%     
==========================================
  Files           5        5              
  Lines         416      422       +6     
==========================================
+ Hits          396      402       +6     
  Misses         20       20
Impacted Files Coverage Δ
src/OffsetArrays.jl 98.22% <100.00%> (+0.01%) ⬆️
src/axes.jl 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e9f33f0...50dc7fb. Read the comment docs.

johnnychen94
johnnychen94 approved these changes Jun 14, 2021
View reviewed changes
Copy link
Member

@johnnychen94 johnnychen94 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The type piracy here is concerning; but seems no good way to appropriately avoid it.

src/OffsetArrays.jl Show resolved Hide resolved
@jishnub
Copy link
Member Author

jishnub commented Jun 14, 2021

Yes, I'll make a PR to move these methods to Base. In the meanwhile it's perhaps better to have them here to obtain correct results.

I'm also a bit concerned that this is quickly becoming a minefield of ambiguities. Not sure if there's an easy way around that either.

@johnnychen94
Copy link
Member

johnnychen94 commented Jun 14, 2021 •
edited
Loading

Aqua only test if OffsetArrays introduces new ambiguity. We won't know if loading other packages introducing new getindex ambiguity until someone reports it...

Another possible downside of this piracy is that it would slow down the package loading via method invalidation. I'll rebase this branch on #241 to see if there're new invalidations. I just merge #241 into master, can you rebase on it and retrigger the CI to see if there're new invalidations? I bet there will be.

@jishnub jishnub merged commit 7a43b4c into JuliaArrays:master Jun 14, 2021
@jishnub jishnub deleted the getindexranges branch June 14, 2021 11:23
@jishnub jishnub mentioned this pull request Jun 14, 2021
Merged
@johnnychen94 johnnychen94 mentioned this pull request Jun 18, 2021
Closed
@jishnub jishnub mentioned this pull request Jun 20, 2021
Merged
jishnub added a commit to JuliaLang/julia that referenced this pull request Mar 9, 2024
@jishnub
Add methods to index identityUnitRange/Slice with another IdentityUni…
6f8ba49 
…tRange (#41224)

Adding these methods lets `OffsetArrays` define
`getindex(::AbstractUnitRange, ::IdentityUnitRange)` without
ambiguities. This is in the domain of sanctioned type-piracy, as the
result is an offset range in general and cannot be represented correctly
using `Base` types.

Re: JuliaArrays/OffsetArrays.jl#244

cc: @johnnychen94 

Edit: this also fixes an indexing bug in `IdentityUntiRange`:
master
```julia
julia> r = Base.IdentityUnitRange(-3:3)
Base.IdentityUnitRange(-3:3)

julia> r[2]
2

julia> r[big(2)]
-2
```

Co-authored-by: jishnub <jishnub@users.noreply.github.com>
mkitti pushed a commit to mkitti/julia that referenced this pull request Apr 13, 2024
@jishnub @mkitti
Add methods to index identityUnitRange/Slice with another IdentityUni…
23ed340 
…tRange (JuliaLang#41224)

Adding these methods lets `OffsetArrays` define
`getindex(::AbstractUnitRange, ::IdentityUnitRange)` without
ambiguities. This is in the domain of sanctioned type-piracy, as the
result is an offset range in general and cannot be represented correctly
using `Base` types.

Re: JuliaArrays/OffsetArrays.jl#244

cc: @johnnychen94 

Edit: this also fixes an indexing bug in `IdentityUntiRange`:
master
```julia
julia> r = Base.IdentityUnitRange(-3:3)
Base.IdentityUnitRange(-3:3)

julia> r[2]
2

julia> r[big(2)]
-2
```

Co-authored-by: jishnub <jishnub@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@johnnychen94 johnnychen94 johnnychen94 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jishnub @johnnychen94